-
Notifications
You must be signed in to change notification settings - Fork 168
Adding XLinear_Velocity interpolator #2461
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Following the discussion around zonal and meridional spherical conversion in #2455
To confirm unit conversion works
Temporarily, as #2461 will move this unit conversion to the vector-interpolators
|
Making sure I understand.. This PR would remove the fallback to component-wise interpolation and require a vector field interpolator to be defined ? |
No, it would make it more explicit. I moved the code we already had into its own Parcels/src/parcels/interpolators.py Lines 714 to 730 in f1ce1e8
The advantage is that users can now more easily see how velocities are interpolated (since Interplators are much more public-facing), and that they can also more easily make adaptations (such as e.g. changing from spherical conversion to ellipsoid conversion in lines 723/724) OK? |
VeckoTheGecko
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just did a full review - looks good! One question out of my own curiousity
| vector_interp_method: Callable | None = None, | ||
| ): | ||
| _assert_str_and_python_varname(name) | ||
| if vector_interp_method is None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't this require that users provide a vector interpolation method here and not allow fall back onto component wise interpolation ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, but the vector_interp_method can fall back on the component wise interpolation. That's exactly what's done in
Parcels/src/parcels/interpolators.py
Lines 714 to 730 in f1ce1e8
| def Ux_Velocity( | |
| particle_positions: dict[str, float | np.ndarray], | |
| grid_positions: dict[_UXGRID_AXES, dict[str, int | float | np.ndarray]], | |
| vectorfield: VectorField, | |
| ): | |
| """Interpolation kernel for Vectorfields of velocity on a UxGrid.""" | |
| u = vectorfield.U._interp_method(particle_positions, grid_positions, vectorfield.U) | |
| v = vectorfield.V._interp_method(particle_positions, grid_positions, vectorfield.V) | |
| if vectorfield.grid._mesh == "spherical": | |
| u /= 1852 * 60 * np.cos(np.deg2rad(particle_positions["lat"])) | |
| v /= 1852 * 60 | |
| if "3D" in vectorfield.vector_type: | |
| w = vectorfield.W._interp_method(particle_positions, grid_positions, vectorfield.W) | |
| else: | |
| w = 0.0 | |
| return u, v, w |
This PR adds an explicit XLinear_Velocity interpolator, effectively moving the code from inside field.py. This improves transparency what is being done in interpolation.
Parcels/src/parcels/_core/field.py
Lines 307 to 315 in eeedce8
First of all, do you agree this would be a good change?
A few more more things to do
Grid_VelcoityandXLinear_Velocitywould be based on grid metadata, in FieldSet creation. @VeckoTheGecko, could you help?_vector_interp_method=None